-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add snippets for all form types and add docs links #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nits, but I think this is good
@@ -181,7 +181,7 @@ export const App = () => { | |||
Coder | |||
</a> | |||
<a | |||
href="https://coder.com" | |||
href="https://coder.com/docs/admin/templates/extending-templates/parameters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: for accessibility, whenever you have an external link, you want to add some screen reader text to make sure saying that the link opens in a new tab
It doesn't have to be much. It can be something like:
<span className="sr-only> (link opens in new tab)</span>
src/client/Editor.tsx
Outdated
const nextInOrder = | ||
parameters.reduce( | ||
(highestOrder, parameter) => | ||
highestOrder < parameter.order ? parameter.order : highestOrder, | ||
0, | ||
) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'm generally not a fan of .reduce
in JS, but I think it's fine here, since we're just working with primitive values, and actually have true function purity
Still, I'm wondering if we could do this:
const nextInOrder = 1 + Math.max(0, ...parameters.map((p) => p.order));
Feels like the Math.max
is more clear about the intent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware that Math.max
was a variadic function!
src/client/Editor.tsx
Outdated
setCode( | ||
`${code.trimEnd()}\n\n${snippet(nameCount > 0 ? `${name}-${nameCount}` : name, nextInOrder)}\n`, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this line of code is doing a lot, especially since we have to mindful of all the newlines. Could we refactor it to split up the steps?
const nextInOrder = 1 + Math.max(0, ...parameters.map((p) => p.order));
const newName = nameCount > 0 ? `${name}-${nameCount}` : name;
const newSnippet = snippet(newName, nextInOrder);
setCode(`${code.trimEnd()}\n\n${newSnippet}\n`);
({ name, label, icon: Icon, snippet }, index) => ( | ||
<DropdownMenuItem | ||
key={index} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DropdownMenuItem
looks stateful, which means that using array indices could eventually scramble up state during JSX reconciliation if we're not careful
If names aren't unique, I feel like it'd be better to find a way to generate a unique ID as a snippet is getting added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snippets array doesn't change, it's a set list the user can pick from. Unless you're talking about onAddSnippet
depending on state and that causing some weird interaction I'm not aware of I think it's fine.
@@ -239,7 +239,9 @@ const PreviewEmptyState = () => { | |||
</p> | |||
</div> | |||
<a | |||
href="#todo" | |||
href="https://coder.com/docs/admin/templates/extending-templates/parameters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same note here about links that open in new tabs
src/client/snippets.ts
Outdated
type Snippet = { | ||
name: string; | ||
label: string; | ||
icon: typeof RadioIcon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this can be redefined via the LucideIcon
type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had tried that but I think I used the Icon
type which didn't work. Will give LucideIcon
a go
src/client/snippets.ts
Outdated
name = "an-input" | ||
description = "What is the name of your project?" | ||
order = 4 | ||
export type SnippetFunc = (name?: string, order?: number) => string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is making both parameters optional really the right move? I feel like you'd almost always want to have these be defined, and in that case, making them required reduces the risk of misuse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I think I had some clever idea around this but it never actually came to fruition.
src/client/Editor.tsx
Outdated
const copyTimeoutId = useRef<ReturnType<typeof setTimeout> | undefined>( | ||
undefined, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this ref here. The timeout ID state can live entirely in the effect
No description provided.